Feature/project-time-travel #2329
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 ESLint
npm warn config production Use WalkthroughThis pull request introduces a comprehensive set of changes across multiple files in the project, focusing on enhancing the project map and time travel functionality. The modifications include adding a new React functional component Changes
Possibly related PRs
Suggested Labels
Suggested Reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
- enables switching between time travel and field data modes - 'Satellite Analysis' is currently disabled
- Add error handling with ErrorHandlingContext - Add loading states and visual feedback - Improve data validation and map initialization - Add configurable year/source selection dropdowns - Add view state syncing with main map - Optimize performance with proper cleanup and memoization - Improve TypeScript types and error constants
- mount TimeTravel only when selectedTab = timeTravel for the first time - unmount TimeTravel only while moving off the single project page - prevent unnecessary reloads of tiles in comparison maps
mohitb35
force-pushed
the
feature/project-time-travel
branch
from
805bb7f to
f36ccf7
Compare
…nd zoomInToProjectSite
…ering - plant locations not shown if selected tab is not field data
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/features/projectsV2/TimeTravelDropdown/index.tsx (2)
51-52: Consider memoizing the helper function.The
isOptionSelectedfunction could benefit from memoization to prevent unnecessary re-renders when used in the render logic.-const isOptionSelected = (option: string, selectedValue: string): boolean => - option.toLowerCase() === selectedValue.toLowerCase(); +const isOptionSelected = React.useCallback( + (option: string, selectedValue: string): boolean => + option.toLowerCase() === selectedValue.toLowerCase(), + [] +);
63-69: Improve type safety for translations.The translation key and rich text parameters lack type safety. Consider creating a type-safe translation setup.
-{tTimeTravel.rich('sourceAttributionLabel', { +type TimeTravelTranslations = { + sourceAttributionLabel: { + year: string; + source: string; + highlight: (chunks: string) => JSX.Element; + }; +}; + +{tTimeTravel.rich<keyof TimeTravelTranslations>('sourceAttributionLabel', { year: selectedYear, source: SOURCE_LABELS[selectedSource], highlight: (chunks) => ( <span className={styles.highlighted}>{chunks}</span> ), })}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/features/projectsV2/TimeTravelDropdown/index.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (1)
src/features/projectsV2/TimeTravelDropdown/index.tsx (1)
14-23: LGTM! Well-structured interface definition.The
TimeTravelDropdownPropsinterface is well-defined with proper TypeScript types and clear prop names.
- for improved modularity and readability
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
src/utils/mapsV2/timeTravel.ts (3)
24-42: Add error handling and consider configuration-driven approach.The function could benefit from:
- Error handling for invalid or missing configuration.
- Moving hardcoded values to a configuration file.
+const CONFIG_ERROR = 'Invalid or missing time travel configuration'; + export const getTimeTravelConfig = (): TimeTravelConfig => { const result: TimeTravelConfig = {}; + + if (!timeTravelConfig) { + throw new Error(CONFIG_ERROR); + } for (const source of SOURCE_NAMES) { const sourceData = timeTravelConfig[source]?.wayback; - if (!sourceData) continue; + if (!sourceData) { + console.warn(`No wayback data found for source: ${source}`); + continue; + }
47-52: Add input validation for URL format.The function should validate that the input URL contains the expected WMTS format parameters before performing replacements.
const convertToZYXFormat = (url: string): string => { + const requiredParams = ['{level}', '{row}', '{col}']; + if (!requiredParams.every(param => url.includes(param))) { + throw new Error('Invalid WMTS URL format'); + } + return url .replace('{level}', '{z}') .replace('{row}', '{y}') .replace('{col}', '{x}'); };
59-81: Add input validation and consider performance optimization.The function should validate input and could be optimized for large datasets.
const getLatestByYear = (items: WaybackItem[]): SingleYearTimeTravelData[] => { + if (!Array.isArray(items)) { + throw new Error('Invalid input: expected array of WaybackItem'); + } + + // Pre-sort items by releaseDatetime for better performance with large datasets + items.sort((a, b) => b.releaseDatetime - a.releaseDatetime); + const intermediate = items.reduce< Record<string, { rasterUrl: string; timestamp: number }> >((acc, item) => { const year = new Date(item.releaseDatetime).getFullYear().toString(); - const existing = acc[year]; - - if (!existing || item.releaseDatetime > existing.timestamp) { + if (!acc[year]) { acc[year] = { rasterUrl: convertToZYXFormat(item.itemURL), timestamp: item.releaseDatetime, }; }src/features/projectsV2/ProjectsMap/TimeTravel/index.tsx (4)
27-43: Move configuration constants to a dedicated file.Consider extracting the map configuration constants and error codes to a dedicated configuration file for better maintainability.
Create a new file
src/features/projectsV2/ProjectsMap/TimeTravel/config.ts:export const MAP_CONFIG = { EMPTY_STYLE: { version: 8, sources: {}, layers: [], }, DEFAULT_SOURCE: 'esri', DEFAULT_YEARS: { BEFORE: '2014', AFTER: '2021', }, }; export const MAP_ERROR_CODES = { INITIALIZATION: 'MAP_INIT_ERROR', DATA_MISSING: 'MAP_DATA_ERROR', LAYER_LOAD: 'MAP_LAYER_ERROR', INVALID_SOURCE: 'MAP_SOURCE_ERROR', INVALID_YEAR: 'MAP_YEAR_ERROR', } as const;
66-71: Combine related state variables.Consider using a single state object for loading states and dropdown states to reduce the number of state variables.
- const [isLoading, setIsLoading] = useState(true); - const [beforeLoaded, setBeforeLoaded] = useState(false); - const [afterLoaded, setAfterLoaded] = useState(false); - const [beforeDropdownOpen, setBeforeDropdownOpen] = useState(false); - const [afterDropdownOpen, setAfterDropdownOpen] = useState(false); + const [mapState, setMapState] = useState({ + isLoading: true, + beforeLoaded: false, + afterLoaded: false, + dropdowns: { + before: false, + after: false + } + });
109-146: Extract validation logic to a separate utility function.The validation logic in
validateDatais complex and could be moved to a separate utility function for better maintainability and reusability.Create a new file
src/features/projectsV2/ProjectsMap/TimeTravel/utils.ts:export const validateTimeTravelData = ( sitesGeoJson: FeatureCollection, timeTravelConfig: ProjectTimeTravelConfig, selectedSourceBefore: SourceName, selectedSourceAfter: SourceName, selectedYearBefore: string, selectedYearAfter: string ) => { if (!sitesGeoJson?.features) { throw new Error('Invalid or missing GeoJSON data'); } // ... rest of validation logic };
484-484: Enhance loading overlay with progress information.Consider adding more detailed loading information and possibly a progress indicator.
-{isLoading && <div className={styles.loadingOverlay}>Loading...</div>} +{isLoading && ( + <div className={styles.loadingOverlay}> + <div className={styles.loadingSpinner} /> + <div className={styles.loadingText}> + Loading satellite imagery... + {!beforeLoaded && <div>Initializing before map...</div>} + {!afterLoaded && <div>Initializing after map...</div>} + </div> + </div> +)}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/features/projectsV2/ProjectsMap/TimeTravel/index.tsx(1 hunks)src/utils/mapsV2/timeTravel.ts(1 hunks)
🔇 Additional comments (3)
src/utils/mapsV2/timeTravel.ts (2)
97-98: Address TODO comment regarding cache time.The cache time is temporarily set to 5 minutes. Consider:
- Implementing a proper cache invalidation strategy.
- Making the cache duration configurable based on data update frequency.
Would you like me to help implement a more robust caching strategy?
106-106: Address TODO comment regarding zoom level.The zoom level is hardcoded to 13. Consider:
- Making it configurable based on project requirements.
- Documenting why this specific zoom level was chosen.
Would you like me to help determine and implement the optimal zoom level configuration?
src/features/projectsV2/ProjectsMap/TimeTravel/index.tsx (1)
292-292: Update ESRI attribution text.Replace the generic 'layer attribution' with ESRI's official attribution text.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/features/projectsV2/ProjectsMap/TimeTravel/index.tsx (3)
126-128: Enhance error messages for better user experience.Consider making error messages more user-friendly and actionable. For example:
- `Year ${selectedYearBefore} not found in source ${selectedSourceBefore}` + `Selected year ${selectedYearBefore} is not available for ${selectedSourceBefore}. Please choose a different year.` - `Year ${selectedYearAfter} not found in source ${selectedSourceAfter}` + `Selected year ${selectedYearAfter} is not available for ${selectedSourceAfter}. Please choose a different year.`Also applies to: 135-137
229-235: Enhance cleanup logic for comparison instance.The cleanup function should also reset the loading state and loaded states to ensure proper re-initialization if the component remounts.
return () => { beforeMap.remove(); afterMap.remove(); if (compareRef.current) { compareRef.current.remove(); + compareRef.current = null; } + setIsLoading(true); + setBeforeLoaded(false); + setAfterLoaded(false); };
416-416: Remove unnecessary spread operator usage.The spread operator usage here is unnecessary as timeTravelConfig is already an object with the required structure.
- { ...timeTravelConfig, sources: timeTravelConfig.sources }, + timeTravelConfig,Also applies to: 426-426
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/features/projectsV2/ProjectsMap/TimeTravel/index.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (5)
src/features/projectsV2/ProjectsMap/TimeTravel/index.tsx (5)
1-44: Well-structured imports and constants!The code demonstrates good TypeScript practices with proper type imports and const assertions for error codes.
191-242: Add error boundary for map initialization.The map initialization could benefit from being wrapped in an error boundary component to handle rendering failures gracefully.
317-317: Update attribution to ESRI's official text.Replace the generic 'layer attribution' with ESRI's official attribution text.
484-503: Add click outside handling for dropdowns.The dropdowns should close when clicking outside of them.
89-100: Consider more robust state initialization.The current initialization of selected years and sources with default values could cause issues if the available data doesn't include these specific years (2014, 2021). Consider initializing with undefined and adding validation logic, or ensure these defaults always exist in the data.
✅ Verification successful
Default year values are properly aligned with data boundaries
The default years (2014 and 2021) are safe to use as they match the actual boundaries of the available time travel data, with 2014 being the earliest available year and 2021 being the latest, as confirmed in the time travel configuration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if default years exist in time travel configurations rg -l "2014|2021" --type jsonLength of output: 363
Script:
#!/bin/bash # Check the content of relevant JSON files echo "=== Content of time-travel.json ===" cat public/data/maps/time-travel.json | jq '.' echo -e "\n=== Content of config.json ===" cat src/utils/mapsV2/config.json | jq '.' # Also find where these default years are defined echo -e "\n=== Default years definition ===" rg "DEFAULT_(BEFORE|AFTER)_YEAR.*=.*\b(2014|2021)\b" -A 1 -B 1Length of output: 70645
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/utils/mapsV2/timeTravel.ts (2)
19-21: Consider moving WMTS base URLs to configuration.The WMTS base URL is currently hardcoded. Consider moving it to the
time-travel.jsonconfiguration file for better maintainability and environment-specific configurations.
59-81: Add input validation and improve documentation.The data processing logic is complex and could benefit from:
- Input validation for the items array
- Additional comments explaining the reduction logic
const getLatestByYear = (items: WaybackItem[]): SingleYearTimeTravelData[] => { + if (!Array.isArray(items)) { + throw new Error('Invalid input: expected array of WaybackItem'); + } + + // Group items by year, keeping only the latest entry for each year const intermediate = items.reduce< Record<string, { rasterUrl: string; timestamp: number }> >((acc, item) => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/utils/mapsV2/timeTravel.ts(1 hunks)
🧰 Additional context used
📓 Learnings (1)
src/utils/mapsV2/timeTravel.ts (2)
Learnt from: mohitb35
PR: Plant-for-the-Planet-org/planet-webapp#2329
File: src/utils/mapsV2/timeTravel.ts:100-117
Timestamp: 2025-01-27T08:15:26.468Z
Learning: When validating GeoJSON Point coordinates at runtime, check if coordinates is an array and has exactly 2 elements (X,Y coordinates). The TypeScript Point type handles null checks, while the Position type allows for an optional third Z coordinate according to the GeoJSON spec.
Learnt from: mohitb35
PR: Plant-for-the-Planet-org/planet-webapp#2329
File: src/utils/mapsV2/timeTravel.ts:100-117
Timestamp: 2025-01-27T08:10:51.101Z
Learning: In geospatial applications, while TypeScript types (like `Point` from `geojson`) provide compile-time safety, runtime validation of coordinates is still necessary when dealing with external data sources to ensure data integrity and prevent runtime errors.
🔇 Additional comments (6)
src/utils/mapsV2/timeTravel.ts (6)
44-52: LGTM! Well-documented utility function.The URL format conversion function is clear, concise, and properly documented.
83-91: LGTM! Well-structured type definitions.The type definitions are clear, properly exported, and follow TypeScript best practices.
97-98: Address TODO comment regarding cache time.The cache time is temporarily set to 5 minutes instead of 24 hours. Please confirm and update the final cache duration.
113-113: Address TODO comment regarding zoom level.The zoom level is hardcoded to 13. Please confirm and document the appropriate zoom level for the application's needs.
100-106: LGTM! Proper coordinate validation.The coordinate validation follows best practices for GeoJSON Point validation at runtime, as discussed in previous reviews.
24-42: Add error handling for malformed configuration data.The function should validate the imported configuration data and handle potential errors gracefully. Consider adding try-catch blocks and data validation.
export const getTimeTravelConfig = (): TimeTravelConfig => { + if (!timeTravelConfig || typeof timeTravelConfig !== 'object') { + console.error('Invalid time travel configuration'); + return {}; + } const result: TimeTravelConfig = {}; // ... rest of the function
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/features/projectsV2/TimeTravelDropdown/index.tsx (4)
1-1: Consider using path aliases for imports.The imports for icons use multiple parent directory references (
../../../../). Consider using path aliases (e.g.,@assets/images/icons/...) to improve maintainability and readability.Also applies to: 6-8
41-44: Add error handling and memoize callbacks.The change handlers should include error handling and be memoized to prevent unnecessary re-renders.
+import React, { useState, useCallback } from 'react'; + -const handleChangeYear = (year: string) => { +const handleChangeYear = useCallback((year: string) => { + try { setSelectedYear(year); onYearChange(year); + } catch (error) { + console.error('Error updating year:', error); + } -}; +}, [onYearChange]); -const handleChangeSource = (source: SourceName) => { +const handleChangeSource = useCallback((source: SourceName) => { + try { setSelectedSource(source); onSourceChange(source); + } catch (error) { + console.error('Error updating source:', error); + } -}; +}, [onSourceChange]);Also applies to: 46-49
83-98: Add type safety to event handlers.The click handlers should include proper event types for better type safety.
-onClick={() => handleChangeYear(year)} +onClick={(e: React.MouseEvent<HTMLTimeElement>) => handleChangeYear(year)}
101-113: Add proper ARIA attributes to source list items.The source list items should have proper ARIA attributes for better accessibility.
<li key={index} + role="option" + aria-selected={isOptionSelected(source, selectedSource)} onClick={() => handleChangeSource(source)} className={`${ isOptionSelected(source, selectedSource) ? styles.selectedMenuItem : styles.unselectedMenuItem }`} > {SOURCE_LABELS[source]} </li>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
public/static/locales/en/projectDetails.jsonis excluded by!**/*.json
📒 Files selected for processing (1)
src/features/projectsV2/TimeTravelDropdown/index.tsx(1 hunks)
🧰 Additional context used
📓 Learnings (1)
src/features/projectsV2/TimeTravelDropdown/index.tsx (1)
Learnt from: mohitb35
PR: Plant-for-the-Planet-org/planet-webapp#2329
File: src/features/projectsV2/TimeTravelDropdown/index.tsx:37-39
Timestamp: 2025-01-23T09:31:12.093Z
Learning: The TimeTravelDropdown component in src/features/projectsV2/TimeTravelDropdown/index.tsx follows an uncontrolled component pattern where defaultYear, defaultSource and isOpen props are used for initial values only, with callbacks to notify parent of changes.
🔇 Additional comments (3)
src/features/projectsV2/TimeTravelDropdown/index.tsx (3)
14-23: LGTM! Well-structured props interface.The
TimeTravelDropdownPropsinterface is well-defined with proper types and clear naming.
121-121: LGTM! Proper export statement.The default export follows React conventions.
10-12: Verify completeness of SOURCE_LABELS.The
SOURCE_LABELSobject only includes 'esri'. Ensure this covers all possible values ofSourceNametype to prevent potential runtime errors.✅ Verification successful
SOURCE_LABELS is complete and type-safe
The
SOURCE_LABELSobject correctly includes all possible values ofSourceNametype, which currently only allows 'esri' as a valid source. No changes needed.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for SourceName type definition and its usage ast-grep --pattern 'type SourceName = $$$' rg -A 5 'SourceName'Length of output: 6024
Script:
#!/bin/bash # Search for SOURCE_NAMES constant definition ast-grep --pattern 'const SOURCE_NAMES = $$$' rg -A 2 'SOURCE_NAMES ='Length of output: 337
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/utils/mapsV2/timeTravel.ts (3)
20-22: Consider moving the ESRI URL to environment variables.The ESRI map tiles URL should be configurable through environment variables to support different environments and make it easier to update.
+// In .env file +NEXT_PUBLIC_ESRI_MAPTILES_URL=https://wayback.maptiles.arcgis.com/arcgis/rest/services/World_Imagery/WMTS/1.0.0/default028mm/MapServer/tile +// In constants file +const ESRI_MAPTILES_URL = process.env.NEXT_PUBLIC_ESRI_MAPTILES_URL; const SOURCE_BASE_URLS: Record<SourceName, string> = { - esri: 'https://wayback.maptiles.arcgis.com/arcgis/rest/services/World_Imagery/WMTS/1.0.0/default028mm/MapServer/tile', + esri: ESRI_MAPTILES_URL, };
4-5: Track removal of legacy time travel configuration.This code segment and the associated JSON file are marked for removal. Consider creating a tracking issue to ensure this technical debt is addressed.
Would you like me to create an issue to track the removal of the legacy time travel configuration?
Also applies to: 24-44
61-83: Enhance type safety in data transformation.While the implementation is efficient, we can improve type safety by:
- Using a more specific type for the timestamp
- Adding type guard for the intermediate record
+type TimestampMS = number; +interface YearData { + rasterUrl: string; + timestamp: TimestampMS; +} -const getLatestByYear = (items: WaybackItem[]): SingleYearTimeTravelData[] => { +const getLatestByYear = (items: WaybackItem[]): SingleYearTimeTravelData[] => { const intermediate = items.reduce< - Record<string, { rasterUrl: string; timestamp: number }> + Record<string, YearData> >((acc, item) => { const year = new Date(item.releaseDatetime).getFullYear().toString(); const existing = acc[year];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/features/common/Layout/ProjectPropsContext.tsx(3 hunks)src/features/projects/components/maps/Project.tsx(2 hunks)src/utils/mapsV2/timeTravel.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/features/projects/components/maps/Project.tsx
- src/features/common/Layout/ProjectPropsContext.tsx
🧰 Additional context used
📓 Learnings (1)
src/utils/mapsV2/timeTravel.ts (2)
Learnt from: mohitb35
PR: Plant-for-the-Planet-org/planet-webapp#2329
File: src/utils/mapsV2/timeTravel.ts:100-117
Timestamp: 2025-01-27T08:15:26.468Z
Learning: When validating GeoJSON Point coordinates at runtime, check if coordinates is an array and has exactly 2 elements (X,Y coordinates). The TypeScript Point type handles null checks, while the Position type allows for an optional third Z coordinate according to the GeoJSON spec.
Learnt from: mohitb35
PR: Plant-for-the-Planet-org/planet-webapp#2329
File: src/utils/mapsV2/timeTravel.ts:100-117
Timestamp: 2025-01-27T08:10:51.101Z
Learning: In geospatial applications, while TypeScript types (like `Point` from `geojson`) provide compile-time safety, runtime validation of coordinates is still necessary when dealing with external data sources to ensure data integrity and prevent runtime errors.
🔇 Additional comments (5)
src/utils/mapsV2/timeTravel.ts (5)
46-54: LGTM! Well-documented utility function.The URL format conversion function is clear, focused, and properly documented.
85-93: LGTM! Well-structured type definitions.The type definitions are clear, properly leveraging TypeScript features for type safety.
115-115: Verify and document the zoom level choice.The zoom level is hardcoded to 13 with a TODO comment. Please verify if this is the optimal zoom level for the use case and document the reasoning.
102-126: LGTM! Robust implementation with proper validation.The function implementation includes:
- Proper coordinate validation (as discussed in previous reviews)
- Error handling
- Null safety checks
99-100: Update cache time configuration.The cache time is temporarily set to 5 minutes instead of the intended 24 hours. Update this once testing is complete.
- // TODO - change temp cache time - const CACHE_TIME_IN_SECONDS = 60 * 5; /* 60 * 60 * 24 */ + const CACHE_TIME_IN_SECONDS = 60 * 60 * 24; // 24 hours
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
src/utils/mapsV2/timeTravel.ts (6)
20-22: Consider moving the ESRI URL to environment variables.The hardcoded ESRI map tiles URL should be moved to environment variables for better configuration management and security.
-const SOURCE_BASE_URLS: Record<SourceName, string> = { - esri: 'https://wayback.maptiles.arcgis.com/arcgis/rest/services/World_Imagery/WMTS/1.0.0/default028mm/MapServer/tile', -}; +const SOURCE_BASE_URLS: Record<SourceName, string> = { + esri: process.env.NEXT_PUBLIC_ESRI_WAYBACK_TILES_URL || '', +};
24-25: Enhance the TODO comment with removal criteria.The comment should specify when or under what conditions this code can be removed.
-// To remove once we remove the old maps code +// TODO: Remove this function and associated JSON config when the legacy maps implementation is deprecated (tracked in issue #XXXX)
26-44: Add error handling for malformed configuration data.The function should handle potential errors in the JSON configuration data structure.
export const getTimeTravelConfig = (): TimeTravelConfig => { const result: TimeTravelConfig = {}; for (const source of SOURCE_NAMES) { + try { const sourceData = timeTravelConfig[source]?.wayback; if (!sourceData) continue; result[source] = []; for (const [year, data] of Object.entries(sourceData)) { if (data?.id && data.id.length > 0) { const url = `${SOURCE_BASE_URLS[source]}/${data.id}/{z}/{y}/{x}`; result[source].push({ year, raster: url }); } } + } catch (error) { + console.error(`Error processing time travel config for source ${source}:`, error); + continue; + } } return result; };
61-83: Enhance type safety for wayback items processing.Consider adding runtime validation for the wayback items and their properties.
const getLatestByYear = (items: WaybackItem[]): SingleYearTimeTravelData[] => { + if (!Array.isArray(items)) { + throw new Error('Expected array of wayback items'); + } + const intermediate = items.reduce< Record<string, { rasterUrl: string; timestamp: number }> >((acc, item) => { + if (!item.releaseDatetime || !item.itemURL) { + console.warn('Skipping invalid wayback item:', item); + return acc; + } + const year = new Date(item.releaseDatetime).getFullYear().toString(); const existing = acc[year];
114-114: Enhance the TODO comment about zoom level.The comment should provide context about why zoom level 13 was chosen and what factors should be considered when updating it.
- 13 //TODO - confirm zoom level and update + 13 // TODO: Validate if zoom level 13 provides optimal balance between detail and performance. + // Consider: tile size, coverage area, and ESRI service limits.
133-137: Enhance error logging with structured error information.The error logging could be more informative by including project ID and structured error details.
} catch (err) { - console.error('Error fetching time travel data:', err); + console.error('Error fetching time travel data:', { + projectId, + error: err instanceof Error ? err.message : String(err), + stack: err instanceof Error ? err.stack : undefined, + }); // Return empty config on error to gracefully degrade return null; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/utils/mapsV2/timeTravel.ts(1 hunks)
🧰 Additional context used
📓 Learnings (1)
src/utils/mapsV2/timeTravel.ts (2)
Learnt from: mohitb35
PR: Plant-for-the-Planet-org/planet-webapp#2329
File: src/utils/mapsV2/timeTravel.ts:100-117
Timestamp: 2025-01-27T08:15:26.468Z
Learning: When validating GeoJSON Point coordinates at runtime, check if coordinates is an array and has exactly 2 elements (X,Y coordinates). The TypeScript Point type handles null checks, while the Position type allows for an optional third Z coordinate according to the GeoJSON spec.
Learnt from: mohitb35
PR: Plant-for-the-Planet-org/planet-webapp#2329
File: src/utils/mapsV2/timeTravel.ts:100-117
Timestamp: 2025-01-27T08:10:51.101Z
Learning: In geospatial applications, while TypeScript types (like `Point` from `geojson`) provide compile-time safety, runtime validation of coordinates is still necessary when dealing with external data sources to ensure data integrity and prevent runtime errors.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (2)
src/utils/mapsV2/timeTravel.ts (2)
46-54: LGTM! Well-documented utility function.The URL format conversion function is clear, focused, and properly documented.
85-93: LGTM! Well-structured type definitions.The types provide good type safety and are properly exported for reuse.
Pending
Cleanup
Changes to load time travel data by hardcoded values using
getTimeTravelConfig()